Merged
Conversation
|
|
View your CI Pipeline Execution ↗ for commit 75cdca5
☁️ Nx Cloud last updated this comment at |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/changeset-preview/upsert-pr-comment.mjs (1)
137-153: Rename status logs from “bundle-size” to “changeset preview”.Current log text is stale and can confuse CI logs when troubleshooting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/changeset-preview/upsert-pr-comment.mjs around lines 137 - 153, Rename the stale "bundle-size" text in the two status log messages to "changeset preview" so CI logs reflect the current feature; update the string literals in the process.stdout.write calls that print `Updated PR #${args.pr} bundle-size comment (${existing.id})` and `Created PR #${args.pr} bundle-size comment (${created?.id ?? 'unknown'})` to use "changeset preview" instead, leaving the surrounding formatting and interpolations (args.pr, existing.id, created?.id) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/changeset-preview/upsert-pr-comment.mjs:
- Line 7: The change introduces a new DEFAULT_MARKER but doesn't handle existing
legacy comments, causing duplicate bot posts; update the upsert logic to search
for and treat legacy and new markers as equivalent by defining an array of
markers (e.g., const MARKERS = [DEFAULT_MARKER, '<!-- old-marker-string -->'])
and use that when finding/updating comments in the functions that create or
replace comments (the code paths that reference DEFAULT_MARKER for matching).
Ensure when updating you prefer the existing legacy comment (replace its body
and then update to the new marker) rather than creating a new comment so the bot
will not post duplicates.
---
Nitpick comments:
In @.github/changeset-preview/upsert-pr-comment.mjs:
- Around line 137-153: Rename the stale "bundle-size" text in the two status log
messages to "changeset preview" so CI logs reflect the current feature; update
the string literals in the process.stdout.write calls that print `Updated PR
#${args.pr} bundle-size comment (${existing.id})` and `Created PR #${args.pr}
bundle-size comment (${created?.id ?? 'unknown'})` to use "changeset preview"
instead, leaving the surrounding formatting and interpolations (args.pr,
existing.id, created?.id) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb535104-daf9-409c-af53-052cb729a523
📒 Files selected for processing (3)
.github/changeset-preview/preview-changeset-versions.mjs.github/changeset-preview/upsert-pr-comment.mjs.github/workflows/changeset-preview.yml
💤 Files with no reviewable changes (1)
- .github/workflows/changeset-preview.yml
| import { parseArgs as parseNodeArgs } from 'node:util' | ||
|
|
||
| const DEFAULT_MARKER = '<!-- bundle-size-benchmark -->' | ||
| const DEFAULT_MARKER = '<!-- changeset-version-preview -->' |
There was a problem hiding this comment.
Handle legacy marker during migration to avoid duplicate bot comments.
After switching markers, existing comments using the old marker are no longer matched, so the first run can create a second preview comment instead of updating the existing one.
Suggested patch
const DEFAULT_MARKER = '<!-- changeset-version-preview -->'
+const LEGACY_MARKERS = ['<!-- bundle-size-benchmark -->']
@@
const existing = comments.find(
(comment) =>
- typeof comment?.body === 'string' && comment.body.includes(args.marker),
+ typeof comment?.body === 'string' &&
+ [args.marker, ...LEGACY_MARKERS].some((marker) =>
+ comment.body.includes(marker),
+ ),
)Also applies to: 123-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/changeset-preview/upsert-pr-comment.mjs at line 7, The change
introduces a new DEFAULT_MARKER but doesn't handle existing legacy comments,
causing duplicate bot posts; update the upsert logic to search for and treat
legacy and new markers as equivalent by defining an array of markers (e.g.,
const MARKERS = [DEFAULT_MARKER, '<!-- old-marker-string -->']) and use that
when finding/updating comments in the functions that create or replace comments
(the code paths that reference DEFAULT_MARKER for matching). Ensure when
updating you prefer the existing legacy comment (replace its body and then
update to the new marker) rather than creating a new comment so the bot will not
post duplicates.
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 1 bumped as dependents. Direct bumps
Dependency bumps (1)
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/changeset-preview/preview-changeset-versions.mjs (2)
113-113:⚠️ Potential issue | 🟠 MajorHandle the promise returned by async
main().Since
mainis async, calling it without handling the returned promise means any errors will result in an unhandled promise rejection. In CI, this may not produce the expected non-zero exit code on failure.Proposed fix
-main() +main().catch((err) => { + console.error(err) + process.exit(1) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/changeset-preview/preview-changeset-versions.mjs at line 113, The async function main() is invoked without handling its returned promise; change the invocation to handle rejections by calling main().catch(err => { processLogger/error output the error and exit with non-zero status }) or wrap the call in a top-level IIFE that awaits main() and catches errors; specifically update the call site of main() so any thrown error is logged (include error details) and process.exit(1) is invoked to ensure CI sees failures.
52-55:⚠️ Potential issue | 🔴 CriticalChange
b.bumptob.typein the sort comparator at line 54.The
@changesets/get-release-planrelease objects use the property nametype(notbump). Sinceb.bumpis undefined,bumpRankalways returns1, causing all packages to sort purely alphabetically rather than by severity (major, minor, patch).Proposed fix
bumps.sort( (a, b) => - bumpRank(b.bump) - bumpRank(a.bump) || a.name.localeCompare(b.name), + bumpRank(b.type) - bumpRank(a.type) || a.name.localeCompare(b.name), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/changeset-preview/preview-changeset-versions.mjs around lines 52 - 55, The sort comparator currently calls bumpRank(b.bump) and bumpRank(a.bump) but release objects use the property name type, so update the comparator in the bumps.sort call to use bumpRank(b.type) and bumpRank(a.type) (keep the existing fallback to a.name.localeCompare(b.name)); this ensures bumpRank is given the correct field and packages are sorted by severity then name.
🧹 Nitpick comments (2)
.github/changeset-preview/preview-changeset-versions.mjs (2)
41-41: Orphaned step comment.The comment "// 6. Diff" is a leftover from the previous implementation that had numbered steps. Consider removing or updating for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/changeset-preview/preview-changeset-versions.mjs at line 41, Remove the orphaned step comment "// 6. Diff" from the file (the leftover numbered-step comment) or replace it with a clear, descriptive comment appropriate for the current implementation so there are no misleading/unused step markers; locate the literal string "// 6. Diff" in .github/changeset-preview/preview-changeset-versions.mjs and delete or update it accordingly.
3-14: Docstring describes the old workflow.The docstring still references "Run
changeset version(mutates package.json files)" and "Diff against the snapshot," but the new implementation usesgetReleasePlandirectly without mutating any files. Consider updating to reflect the new approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/changeset-preview/preview-changeset-versions.mjs around lines 3 - 14, The docstring is outdated: instead of saying the script runs "changeset version" and mutates package.json, update the top comment to describe the current flow that uses getReleasePlan to compute planned releases without mutating files — e.g., snapshot workspace package versions, call getReleasePlan to build the release plan, compare/releases in-memory against the snapshot, and then print or write the markdown summary; mention explicitly that no files are changed and remove or replace the "mutates package.json" and "Diff against the snapshot" wording so the description matches the implementation that uses getReleasePlan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/changeset-preview/preview-changeset-versions.mjs:
- Line 49: Remove the debug print that leaks the raw bumps array: locate the
console.log(bumps) statement and delete it (or replace it with a non-stdout
debug mechanism), ensuring the script no longer writes the raw bumps array to
stdout before emitting the markdown output.
In `@packages/vite-config/package.json`:
- Line 44: Remove the package entry "@tanstack/publish-config" from the
dependencies section and add the same version spec under devDependencies so it's
only installed for development; update the "dependencies" ->
"@tanstack/publish-config" removal and add it to "devDependencies" in
package.json, then reinstall/update the lockfile (npm/yarn/pnpm install) to
reflect the move.
---
Outside diff comments:
In @.github/changeset-preview/preview-changeset-versions.mjs:
- Line 113: The async function main() is invoked without handling its returned
promise; change the invocation to handle rejections by calling main().catch(err
=> { processLogger/error output the error and exit with non-zero status }) or
wrap the call in a top-level IIFE that awaits main() and catches errors;
specifically update the call site of main() so any thrown error is logged
(include error details) and process.exit(1) is invoked to ensure CI sees
failures.
- Around line 52-55: The sort comparator currently calls bumpRank(b.bump) and
bumpRank(a.bump) but release objects use the property name type, so update the
comparator in the bumps.sort call to use bumpRank(b.type) and bumpRank(a.type)
(keep the existing fallback to a.name.localeCompare(b.name)); this ensures
bumpRank is given the correct field and packages are sorted by severity then
name.
---
Nitpick comments:
In @.github/changeset-preview/preview-changeset-versions.mjs:
- Line 41: Remove the orphaned step comment "// 6. Diff" from the file (the
leftover numbered-step comment) or replace it with a clear, descriptive comment
appropriate for the current implementation so there are no misleading/unused
step markers; locate the literal string "// 6. Diff" in
.github/changeset-preview/preview-changeset-versions.mjs and delete or update it
accordingly.
- Around line 3-14: The docstring is outdated: instead of saying the script runs
"changeset version" and mutates package.json, update the top comment to describe
the current flow that uses getReleasePlan to compute planned releases without
mutating files — e.g., snapshot workspace package versions, call getReleasePlan
to build the release plan, compare/releases in-memory against the snapshot, and
then print or write the markdown summary; mention explicitly that no files are
changed and remove or replace the "mutates package.json" and "Diff against the
snapshot" wording so the description matches the implementation that uses
getReleasePlan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd4784f3-8d1f-4b7a-bcff-e1f29837aa3d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/changeset-preview/preview-changeset-versions.mjspackage.jsonpackages/vite-config/package.json
| bumps.push({ ...release, source }) | ||
| } | ||
|
|
||
| console.log(bumps) |
There was a problem hiding this comment.
Remove debug console.log statement.
This outputs the raw bumps array to stdout before the markdown, which will corrupt the output consumed by downstream workflow steps.
Proposed fix
- console.log(bumps)
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log(bumps) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/changeset-preview/preview-changeset-versions.mjs at line 49, Remove
the debug print that leaks the raw bumps array: locate the console.log(bumps)
statement and delete it (or replace it with a non-stdout debug mechanism),
ensuring the script no longer writes the raw bumps array to stdout before
emitting the markdown output.
packages/vite-config/package.json
Outdated
| "node": ">=18" | ||
| }, | ||
| "dependencies": { | ||
| "@tanstack/publish-config": "workspace:*", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether runtime source imports this package
rg -n -C3 --type=ts --type=js --type=tsx --type=mjs \
-g '!**/*.test.*' -g '!**/*.spec.*' \
'\@tanstack/publish-config' packages/vite-config/src packages/vite-config || true
# 2) Show where it is declared today
python - <<'PY'
import json, pathlib
p = pathlib.Path("packages/vite-config/package.json")
data = json.loads(p.read_text())
print("dependencies[`@tanstack/publish-config`] =", data.get("dependencies", {}).get("@tanstack/publish-config"))
print("devDependencies[`@tanstack/publish-config`] =", data.get("devDependencies", {}).get("@tanstack/publish-config"))
PYRepository: TanStack/config
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for imports of `@tanstack/publish-config` in source code (without escaping @)
rg -n -C3 --type=ts --type=js --type=tsx --type=mjs \
-g '!**/*.test.*' -g '!**/*.spec.*' \
'@tanstack/publish-config' packages/vite-config/src packages/vite-config || true
echo "---"
echo "Current package.json declaration:"
python - <<'PY'
import json, pathlib
p = pathlib.Path("packages/vite-config/package.json")
data = json.loads(p.read_text())
deps = data.get("dependencies", {})
devDeps = data.get("devDependencies", {})
print(f"dependencies[`@tanstack/publish-config`] = {deps.get('@tanstack/publish-config', 'NOT FOUND')}")
print(f"devDependencies[`@tanstack/publish-config`] = {devDeps.get('@tanstack/publish-config', 'NOT FOUND')}")
PYRepository: TanStack/config
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the structure and entry point of vite-config package
echo "=== vite-config package.json (relevant fields) ==="
python - <<'PY'
import json, pathlib
p = pathlib.Path("packages/vite-config/package.json")
data = json.loads(p.read_text())
for key in ["main", "exports", "scripts"]:
if key in data:
print(f"{key}: {json.dumps(data[key], indent=2)}")
PY
echo ""
echo "=== Check index/entry point files ==="
ls -la packages/vite-config/ | grep -E "(index|dist)" || echo "No obvious index files"
echo ""
echo "=== Check if `@tanstack/publish-config` is used in build scripts or config ==="
grep -r "publish-config" packages/vite-config/ --include="*.json" --include="*.js" --include="*.ts" 2>/dev/null || echo "Not found in build/config"Repository: TanStack/config
Length of output: 513
🏁 Script executed:
#!/bin/bash
cat -n packages/vite-config/src/index.tsRepository: TanStack/config
Length of output: 4687
Move @tanstack/publish-config to devDependencies.
This package is not imported by any runtime-exported code. The entry point (src/index.ts) and build scripts do not depend on it, so it should not be in dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-config/package.json` at line 44, Remove the package entry
"@tanstack/publish-config" from the dependencies section and add the same
version spec under devDependencies so it's only installed for development;
update the "dependencies" -> "@tanstack/publish-config" removal and add it to
"devDependencies" in package.json, then reinstall/update the lockfile
(npm/yarn/pnpm install) to reflect the move.
lachlancollins
force-pushed
the
always-preview-versions
branch
from
492c730 to
541459a
Compare
📝 WalkthroughWalkthroughThe changeset preview system is refactored from filesystem-based discovery to a release plan-driven approach. The workflow trigger is expanded, PR comment structure is updated with a new header, and a peer dependency is added to vite-config. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vite-config/package.json`:
- Around line 52-55: The package.json currently lists "@tanstack/publish-config"
under peerDependencies even though it's not imported or used at runtime; remove
"@tanstack/publish-config" from the "peerDependencies" section in package.json
and either delete it entirely or add it to "devDependencies" if it is only
needed for build/tooling, ensuring only actual runtime/consumer-provided libs
remain as peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7225a7ca-d377-486a-8374-cc37d1be43fc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/vite-config/package.json
lachlancollins
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit